-
Notifications
You must be signed in to change notification settings - Fork 128
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: allow canvas local config #1496
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
will tag @daibhin too in case of missing context :) |
Size Change: +3.21 kB (+0.11%) Total Size: 2.89 MB
ℹ️ View Unchanged
|
const fps = canvasRecording_client_side?.canvasFps ?? canvasRecording_server_side?.fps ?? 0 | ||
const quality = canvasRecording_client_side?.canvasQuality ?? canvasRecording_server_side?.quality ?? 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be careful about letting users arbitrarily set these two values. We could end up with very large recordings if they set the values too high, particularly the FPS. At the very least I would limit them to sensible ranges and emit a warning log
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah... fair.
have a feeling for reasonable range?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fps... between 0 and 12?
quality between 0 and 1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was very conservative rolling it out initially 😂
# hard coded during beta while we decide on sensible values
"canvasFps": 3 if record_canvas else None,
"canvasQuality": "0.4" if record_canvas else None,
They sound reasonable. Unlikely many people will change the defaults. We can always dial it back in future SDK versions if it causes issues
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 👌
A user wants to be able to dynamically configure canvas recording... so, allow local config